-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: settings form select menu #9179
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Enhanced select components across settings forms with improved search and selection visuals to provide better user experience.
- Added search functionality to currency and address form dropdowns with fixed 220px width to prevent UI shrinking
- Implemented country flags in address form dropdown for better visual identification
- Added
needIconCheck
prop toMenuItemSelect
to optionally hide check icons while maintaining selected state highlighting - Replaced
MenuItem
withMenuItemSelect
across forms for consistent selection feedback - Disabled check icons for boolean fields since they already have true/false icons
6 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
})); | ||
countries.unshift({ | ||
label: 'No country', | ||
value: '', | ||
Icon: IconCircleOff, | ||
}); | ||
const defaultDefaultValue = { | ||
addressStreet1: "''", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: addressStreet1 default value of "''" contains nested quotes which may cause issues. Consider using just '' or an empty string
Thanks for your PR. I will look into it in the coming days, but from my first glance, it looks pretty good ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats. really good job.
I just noticed a new component for countries was designed by someone from the core team. It's called FormCountrySelectInput.
Maybe in the future we should merge the "country picker" types in order to have only one type of this kind.
During my review, I screenshoted the additional changes I noticed on other components in case @Bonapara has something to say. |
Thanks @harshrajeevsingh for your contribution! |
Curious to have your opinion on that @thomtrp (since you designed the new component) |
Closes: #8647 Closes: #8649 **Changes & Why** 1. Added a Search Input to `SettingsDataModelFieldAddressForm` & `SettingsDataModelFieldCurrencyForm` as `Select` component already accepts it as a prop. 2. Gave a fixed width to the dropdown of both the above components to ensure it doesn't shrink on search for the menu items with low word count. 3. Added countries Flag to `SettingsDataModelFieldAddressForm`. 4. Replaced `MenuItem` with `MenuItemSelect` to get the desired highlighted background for the selected item with `IconCheck` to differentiate the current selected item. This is useful across all the select components throughout the app. 5. I realized that in some components we might not need IconCheck and only need a highlighted background for the selected item. For ex: `SettingsDataModelFieldBooleanForm` . Therefore, I created a prop `needIconCheck` with default as true so it doesn't break the existing `MenuItemSelect` and we can pass that prop as false wherever needed. [Screencast from 2024-12-21 12-08-08.webm](https://github.com/user-attachments/assets/4f8070a8-f339-4556-a137-bbbad58b171c)
Closes: twentyhq#8647 Closes: twentyhq#8649 **Changes & Why** 1. Added a Search Input to `SettingsDataModelFieldAddressForm` & `SettingsDataModelFieldCurrencyForm` as `Select` component already accepts it as a prop. 2. Gave a fixed width to the dropdown of both the above components to ensure it doesn't shrink on search for the menu items with low word count. 3. Added countries Flag to `SettingsDataModelFieldAddressForm`. 4. Replaced `MenuItem` with `MenuItemSelect` to get the desired highlighted background for the selected item with `IconCheck` to differentiate the current selected item. This is useful across all the select components throughout the app. 5. I realized that in some components we might not need IconCheck and only need a highlighted background for the selected item. For ex: `SettingsDataModelFieldBooleanForm` . Therefore, I created a prop `needIconCheck` with default as true so it doesn't break the existing `MenuItemSelect` and we can pass that prop as false wherever needed. [Screencast from 2024-12-21 12-08-08.webm](https://github.com/user-attachments/assets/4f8070a8-f339-4556-a137-bbbad58b171c)
Closes: #8647
Closes: #8649
Changes & Why
SettingsDataModelFieldAddressForm
&SettingsDataModelFieldCurrencyForm
asSelect
component already accepts it as a prop.SettingsDataModelFieldAddressForm
.MenuItem
withMenuItemSelect
to get the desired highlighted background for the selected item withIconCheck
to differentiate the current selected item. This is useful across all the select components throughout the app.SettingsDataModelFieldBooleanForm
. Therefore, I created a propneedIconCheck
with default as true so it doesn't break the existingMenuItemSelect
and we can pass that prop as false wherever needed.Screencast.from.2024-12-21.12-08-08.webm